-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MRG, ENH: Add method to project onto max power ori #7883
Conversation
stc_max, directions = stc.project_onto() | ||
flips = np.sign(np.sum(directions * want_nn, axis=1, keepdims=True)) | ||
directions *= flips | ||
assert_allclose(directions, want_nn, atol=1e-6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agramfort here is your proof-by-COBYLA :)
I will have a look later whether this performs like what I wrote yesterday (which is how @sarangnemo implemented it in NutMEG back then) - I guess @larsoner got so excited by the plots I shared that he had to try by himself! 😄 For what it's worth, the code I wrote up yesterday performs close to a scalar beamformer with optimal orientation picking - I have some preliminary code to also allow orientations to vary across time, which might make things more interesting, which I'd be happy to contribute. |
Yes sorry I got excited about figuring out how to take care of the complex-valued case... :)
Eventually we can allow this by allowing In between these two extremes -- i.e., not doing SVD across all time points, or SVD for each individual time point -- things will vary a bit, but I'd be tempted to have people |
Excellent -- in that case this is good to go @agramfort . Once this is merged I'll rebase the beamformer refactoring PR and we can properly test the two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quite skeptical about the API.
You never use the stc.project_onto with directions != None in any example or tutorial.
should directions be a public parameter?
we have stc.normal so should we rather of stc.principal_component?
mne/source_estimate.py
Outdated
stc : instance of SourceEstimate | ||
The projected source estimate. | ||
normals : ndarray, shape (n_vertices, 3) | ||
Only returned if ``normals is None``, the normals computed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only returned if ``normals is None``, the normals computed | |
Only returned if ``directions is None``. |
It was easy enough to add and validation was simple. People could want to for example project onto the max power orientations they got from some other filter. So I don't see a problem/maintenance cost from keeping it.
Starting from scratch I'd actually prefer the API to be |
ok I propose to deprecate VectorSourceEstimate.normal and introduce VectorSourceEstimate.project(directions, src=None, use_cps=True, return_directions=False) where directions can be sounds like a plan? |
I'd rather just always return the directions, return_ kwargs I'd rather leave as a mechanism for backward compatibility. It's very easy to do [0] to get the first output of a function. src is mandatory in both cases to make the sign flip directions not arbitrary |
(deal with sign flip ambiguity in pca) |
ok then to remove return_ param and always return directions.
we have a plan?
|
Yep, I'll push shortly |
Thx @larsoner |
* upstream/master: MRG, ENH: Add method to project onto max power ori (mne-tools#7883) WIP: Warn if untested NIRX device (mne-tools#7905) MRG, BUG: Fix bug with volume morph and subject_to!="fsaverage" (mne-tools#7896) MRG, MAINT: Clean up use of bool, float, int (mne-tools#7917) ENH: Better error message for incompatible Evoked objects (mne-tools#7910) try to fix nullcontext (mne-tools#7908)
* upstream/master: (23 commits) MAINT: Add mne.surface to docstring tests (mne-tools#7930) MRG: Add smoothing controller to TimeViewer for the notebook backend (mne-tools#7928) MRG: TimeViewer matplotlib figure color (mne-tools#7925) fix typos (mne-tools#7924) MRG, ENH: Add method to project onto max power ori (mne-tools#7883) WIP: Warn if untested NIRX device (mne-tools#7905) MRG, BUG: Fix bug with volume morph and subject_to!="fsaverage" (mne-tools#7896) MRG, MAINT: Clean up use of bool, float, int (mne-tools#7917) ENH: Better error message for incompatible Evoked objects (mne-tools#7910) try to fix nullcontext (mne-tools#7908) WIP: Fix Travis (mne-tools#7906) WIP: Prototype of notebook viz (screencast) (mne-tools#7758) MRG, FIX: Speed up I/O tests, mark some slow (mne-tools#7904) Proper attribution for Blender tutorial (mne-tools#7900) MAINT: Check usage [ci skip] (mne-tools#7902) Allow find_bad_channels_maxwell() to return scores (mne-tools#7845) Warn if NIRx directory structure has been modified from original format (mne-tools#7898) Pin pvyista to 0.24.3 (mne-tools#7899) MRG: Add support for reading and writing sufaces to .obj (mne-tools#7824) Fix _auto_topomap_coords docstring. (mne-tools#7895) ...
Useful for:
pick_ori='vector'
solutions to max-power-ori solutionsunit-noise-gain
vector beamformers we'll want to do this (probably)In the beamformer PR, we can also check to see the extent to which
pick_ori='max-power'
agrees withpick_ori='vector'
followed bystc.project_onto()
.